feat(tracemetrics): Use reference map to render equations#112817
Conversation
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…termediate-representation
Accidentally deleted some conditional code
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| ) | ||
| .map((metricQuery, index) => metricQuery.label ?? getVisualizeLabel(index, false)) | ||
| .map((metricQuery, index) => [ | ||
| metricQuery.label ?? getVisualizeLabel(index, false), |
There was a problem hiding this comment.
where the keys are the query label (A, B, etc)
Just noting, similar to #112893 (comment), that IMO it'd be good to eventually have these use some form of unique ID. I've been in a couple meetings lately where it was noted that using the order-dependent labels causes surprising behavior on reordering.
I think this PR is a step in the right direction. 🚀
There was a problem hiding this comment.
So in a previous PR I annotate the metric queries with a slightly more stable ID! It gets auto-generated on load so it's not persisted and resets on refresh (which I wanted to, so I could "compact" the labels by iterating them again from the beginning) and Nick was able to use them for his reordering work. I hope we're okay for now 🙏
#112675
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
Very cool!! Good luck with the merge conflicts though 🥲
…termediate-representation
Sentry Snapshot Testing
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d9f35c5. Configure here.
| const visualize = metricConfig.visualizes[0]!; | ||
| const parsed = parseFunction(visualize.yAxis); | ||
| if (parsed) { | ||
| return `${parsed.name}_if(\`${metricConfig.query}\`,${parsed.arguments.join(',')})`; |
There was a problem hiding this comment.
Queries with spaces break _if format tokenization
Medium Severity
resolveMetricReference generates _if format strings like count_if(`status:ok browser:Chrome`,value,...) where the query is backtick-wrapped. When the query contains spaces (common for multi-condition filters), unresolveExpression fails because the PEG grammar's name rule ([^()\t\n, "]+) stops at spaces, preventing the entire _if(...) call from being parsed as a single TokenFunction token. No single token's text will match the reversed reference map key, so the equation builder displays raw function calls instead of reference labels like A or B.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d9f35c5. Configure here.
There was a problem hiding this comment.
This is okay for now. I've worked around it and I do not want to update the grammar yet. I can keep this in mind to follow up with because I had to remove validity checks against the resolved equation to make this work.
narsaynorath
deleted the
nar/feat/tracemetrics-use-references-as-intermediate-representation
branch
2 participants
Updates the references hook to return a reference map where the keys are the query label (A, B, etc) and the values are the aggregate which represent that query for use in equations. If there's a filter, we convert it to the
_ifformat. e.g. a count on metric A forgeo.country:Canadawould look likeThe equation builder will use the reference map to turn the input expression into the A, B, etc form and render that form. Any updates to that will call a callback that "resolves" the equation to its original aggregate form so we can use it to make downstream
events-timeseriesrequest.I added an effect to pick up on when the reference map changes, because if I'm plotting
A + BandBchanges, I'd want to call the callback so we continue to plot the correct data.